Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

add UnitLowerCholeskyTransform; change default parameterization for AutoMultivariateNormal #2972

Merged
merged 7 commits into from
Nov 28, 2021
Merged

add UnitLowerCholeskyTransform; change default parameterization for AutoMultivariateNormal #2972

merged 7 commits into from
Nov 28, 2021

Conversation

martinjankowiak
Copy link
Collaborator

addresses #2963

questions for reviewers:

  • did i put things in the right files?
  • do i need additional tests?
  • do i need to change any additional autoguide defaults?
  • do i need to do anything on the doc side of things for unit_lower_cholesky?

fritzo
fritzo requested changes Nov 28, 2021
View reviewed changes
Copy link
Member

@fritzo fritzo left a comment •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM except for minor comments.

did i put things in the right files?

👍 yes

do i need additional tests?

Tests already look sufficient.

do i need to change any additional autoguide defaults?

Nope, it looks like AutoMultivariateNormal is the only use of lower_cholesky.

do i need to do anything on the doc side of things for unit_lower_cholesky?

I don't think so, but can you make docs and check that unit_lower_cholesky shows up?

pyro/distributions/transforms/unit_cholesky.py Outdated Show resolved Hide resolved
docs/source/distributions.rst Outdated Show resolved Hide resolved
@martinjankowiak
Copy link
Collaborator Author

unfortunately i'm unable to build the docs:

Warning, treated as error:
autodoc: failed to import module 'multi_mnist' from module 'pyro.contrib.examples'; the following exception was raised:
Traceback (most recent call last):
.........
  File "/Users/mjankowi/pyro/pyro/contrib/examples/util.py", line 15, in MNIST
    ] + datasets.MNIST.mirrors
AttributeError: type object 'MNIST' has no attribute 'mirrors'

tried to upgrade sphinx etc but that didn't help...

@fritzo
Copy link
Member

fritzo commented Nov 28, 2021 •
edited
Loading

i'm unable to build the docs:

Try upgrading torchvision?

pip install -U torch torchvision

IMHO ensuring docs are buildable is higher priority than enhancements

@martinjankowiak
Copy link
Collaborator Author

ah yeah torchvision was it. and with the addition to __all__ i can confirm the docs build and the appropriate entry shows up

fritzo
fritzo approved these changes Nov 28, 2021
View reviewed changes
Copy link
Member

@fritzo fritzo left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍 Thanks for checking docs!

@fritzo fritzo merged commit f9b7d3d into dev Nov 28, 2021
@martinjankowiak martinjankowiak deleted the unitchol branch December 12, 2021 02:49
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@fritzo fritzo fritzo approved these changes

Assignees
No one assigned
Labels
awaiting review easy
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@martinjankowiak @fritzo